Fixed a bug where fleetd could not start on-demand Windows MDM session#48765
Conversation
|
@coderabbitai full review |
|
/agentic_review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR fixes Windows on-demand MDM sync startup failures on newer Windows builds by broadening how orbit identifies Fleet’s MDM enrollment in the registry (accepting non-zero EnrollmentState values like 3, instead of only 1).
Changes:
- Replace the hard-coded
EnrollmentState == 1check with a helper that treats any non-zero enrollment state as enrolled, while still scoping byProviderID == Fleetand validating the subkey GUID format. - Centralize the enrollment GUID validation regex + matching logic in a shared helper (
isActiveFleetEnrollment). - Add unit tests covering accepted/rejected enrollment states and malformed registry subkey names.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| orbit/pkg/update/execwinapi.go | Adds shared helper (isActiveFleetEnrollment) and GUID validation used to identify Fleet’s Windows MDM enrollment. |
| orbit/pkg/update/execwinapi_windows.go | Updates registry scanning logic to use the new helper, allowing non-zero EnrollmentState values. |
| orbit/pkg/update/execwinapi_test.go | Adds unit tests validating the new enrollment-selection rules (including state 3). |
| changes/48760-windows-mdm-enrollment-state | User-visible change entry (content excluded from review per policy). |
Files excluded by content exclusion policy (1)
- changes/48760-windows-mdm-enrollment-state
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughChangesThis change fixes Windows MDM on-demand enrollment detection in fleetd. A new Related issues: Suggested labels: bug, orbit, mdm, windows Suggested reviewers: (based on ownership of orbit/pkg/update package; none specified in provided context) 🐰 A registry key with state of three, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #48765 +/- ##
==========================================
+ Coverage 68.02% 68.03% +0.01%
==========================================
Files 3680 3681 +1
Lines 233959 233970 +11
Branches 12456 12456
==========================================
+ Hits 159145 159178 +33
+ Misses 60496 60481 -15
+ Partials 14318 14311 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
| import ( | ||
| "regexp" | ||
|
|
||
| "github.com/fleetdm/fleet/v4/server/mdm/microsoft/syncml" |
There was a problem hiding this comment.
This import used to be a dependency of windows only (//go:build windows). This makes syncml a dependency of the orbit update package on all platforms.
There was a problem hiding this comment.
@ksykulev nice catch.
I checked the dependencies and we are already pulling it in via: server/fleet → server/mdm/microsoft/syncml
So, there is no new dependency or binary-size impact.
fleetdm#48765) <!-- Add the related story/sub-task/bug number, like Resolves fleetdm#123, or remove if NA --> **Related issue:** Resolves fleetdm#48760 # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## fleetd/orbit/Fleet Desktop Note: changes are Windows only; only verified/tested these items for Linux and Windows - [x] Verified compatibility with the latest released version of Fleet (see [Must rule](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/workflows/fleetd-development-and-release-strategy.md)) - [x] If the change applies to only one platform, confirmed that `runtime.GOOS` is used as needed to isolate changes - [x] Verified that fleetd runs on macOS, Linux and Windows - [x] Verified auto-update works from the released version of component to the new version (see [tools/tuf/test](../tools/tuf/test/README.md)) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed an issue where some Windows MDM enrollment sessions could stay queued or fail to start on certain devices. * Improved detection of active Windows MDM enrollment, helping commands run more reliably when enrollment is already present. * **Tests** * Added coverage for Windows enrollment detection to prevent regressions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Related issue: Resolves #48760
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
fleetd/orbit/Fleet Desktop
Note: changes are Windows only; only verified/tested these items for Linux and Windows
runtime.GOOSis used as needed to isolate changesSummary by CodeRabbit